-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "[CodeGenC] Handle GlobalVar callee as internal function call" #15725
Revert "[CodeGenC] Handle GlobalVar callee as internal function call" #15725
Conversation
Related discussion: #15103 (comment). |
Thank you @junrushao! @Lunderberg I’m sorry for this. Please help take a look at the issue! |
2902c10
to
cf1766c
Compare
Prior to merging I would request that we please include a test or otherwise link to a test that fails so that there is something semi-minimal for which can be used to support the desired use case and prevent the breakage in the future. |
Agreed @csullivan. Given it's a codegen test, it's a bit challenging though to have a minimal test if we don't have unittest infra that could potentially access Metal-capable devices. As suggested by @MasterJH5574 in the prior thread, how about using tests in |
cf1766c
to
45eb402
Compare
As a first step to addressing the Metal codegen errors that required the reversion in apache#15725, parametrizing the unit tests for `allreduce`. While these tests are parametrized with `@tvm.testing.parametrize_targets("cuda", "metal")`, the automatic `tvm.testing.requires_metal` marker inserted for the metal parametrization will cause them to be skipped if the metal runtime is unavailable, which includes the current CI.
* [UnitTest][Metal] Parametrize allreduce GPU tests As a first step to addressing the Metal codegen errors that required the reversion in #15725, parametrizing the unit tests for `allreduce`. While these tests are parametrized with `@tvm.testing.parametrize_targets("cuda", "metal")`, the automatic `tvm.testing.requires_metal` marker inserted for the metal parametrization will cause them to be skipped if the metal runtime is unavailable, which includes the current CI. * Updated filename, device used when testing on metal
This reverts commit [`e88d0d`](apache#15725), which itself reverted [`9ff71f`](apache#15103) for breakages on the metal backend. Now that the CI contains compile-time testing of the metal codegen, the original breakage should be identifiable.
…15835) * [CodeGenC][Redo] Handle GlobalVar callee as internal function call This reverts commit [`e88d0d`](#15725), which itself reverted [`9ff71f`](#15103) for breakages on the metal backend. Now that the CI contains compile-time testing of the metal codegen, the original breakage should be identifiable. * Added codegen metal CI debug print * Print function decl to the argument stream * Remove the codegen metal CI debug print-outs
This PR temporarily reverts a recent change that breaks the Metal backend. Happy to get this commit back once the codegen is fixed.
Reverts #15103